fix(runtime): decide workspace path containment in one realpath space - #2059
Merged
Merged
Conversation
The local workspace executor compared a realpath'd session cwd against a merely resolved candidate path. Any session cwd under a symlink — macOS tmpdirs (`/var` → `/private/var`), or a workspace root organised with symlinks — therefore rejected every legitimate absolute path inside the workspace, so a model writing by absolute path could not touch its own workspace. Canonicalise the candidate through its deepest existing ancestor before the containment check, so both sides live in the realpath space and the target may still be missing. Following the symlinks does not weaken containment: a link inside the cwd pointing out of it now resolves to its outside target and is rejected, including for Write. The write-lock key gets the same canonicalisation, so relative and symlinked-absolute spellings of one file no longer take two different locks. `realpathAllowMissing` moves to path-containment.ts, the owner of the containment invariant, and replaces the two copies in sandbox-boundary-path.ts and filesystem-worker/operations.ts.
Collapsing the two resolvers dropped the check the old existing-path variant ran on its final `fs.realpath` result. The candidate is already canonical, so the two can only diverge when a segment that was missing during canonicalisation becomes a symlink before the realpath — a narrow race, but one the old code caught and the new code returned unchecked to Read, Edit, FormatJson, Glob, and Grep. Restore it by making the containment assertion a named helper both call sites share.
`realpath` fails ENOENT on a symlink whose target does not exist, so `realpathAllowMissing` treated such a link as a plain missing leaf and returned the link's own path. A link inside the workspace pointing at a not-yet-created path outside it therefore read as contained, and a Write through it created the file outside the workspace — verified against the local executor before the fix. Read the link and follow it by hand instead. `realpath` reports a cycle as ELOOP, so each hop consumes one existing link and the walk terminates; the hop cap is a backstop, not the cycle guard. The two other consumers of the helper gain the same accuracy: a dangling link now normalises to the path a write would actually reach, which is the path the sandbox boundary should approve. Covers the helper with direct unit tests — it is now the canonicalisation authority for three subsystems and had none of its own.
The filesystem worker resolves a missing write target by realpathing its parent and then authorising the unresolved candidate. A dangling symlink inside the root therefore passed its boundary check while the write landed on the link's target outside the root — reproduced by handing the worker a request whose boundary names only the workspace-internal link. Reaching it takes a request whose boundary and expectedTarget disagree, which the in-repo client never sends, but the worker is a separate process that re-checks everything precisely so it does not have to trust its caller. Authorise the followed path, the same one `assertTargetUnchanged` already pins the request to.
…hains Neither branch had a test. The chain case documents where the bound actually comes from: the kernel caps its own symlink traversal well below the helper's hop cap, so it answers ELOOP first and the hop cap is a backstop for a filesystem that does not. The contract worth pinning is that the walk terminates by rejecting, not which layer rejects. Also corrects the module docstring: the leaf has not imported only `node:path` for some time.
`writeLockKey` repeated the resolver's root/resolve/canonicalise sequence verbatim. Identical today, so the keys match the resolved paths, but the two spaces drifting apart is exactly what breaks write serialization silently. Split the canonicalisation from the containment assertion so the lock key uses the former and the resolvers add the latter. Also records why the post-realpath assertion in the existing-path resolver must not be deleted: it guards a race no deterministic test can drive, so removing it breaks nothing visible.
Astro-Han
force-pushed
the
fix/runtime-symlink-cwd-containment
branch
from
August 4, 2026 02:19
6fc1a7b to
c2b4468
Compare
Astro-Han
marked this pull request as ready for review
August 4, 2026 02:22
sunheyi6
added a commit
to sunheyi6/maka-agent
that referenced
this pull request
Aug 4, 2026
) conflicts - sandbox-boundary-path: adopt main realpath-space model (no followFinalSymlink) - builtin-tools ApplyPatch adapter: writes follow the canonical target (apache#2059), lstat/delete keep directory-entry semantics via realpathAllowMissing parents - filesystem worker client: entry-mode normalizer for delete/lstat/create; writable roots back to main narrow dirname form - operations: worker write resolves through followed path; delete/lstat operands stay entry-scoped with containment - storage: drop FileSessionStore per main apache#2040; restore editingProtocol persistence in buildSessionHeader + normalizeSessionHeader - tests: merge symlink suites (replace-mode now follow-final), keep PR lstat/ delete entry tests, add editingProtocol persistence regression
sunheyi6
added a commit
to sunheyi6/maka-agent
that referenced
this pull request
Aug 4, 2026
…tingProtocol persistence - filesystem-worker: create-mode parents test uses mode create (plain writes now require the parent in realpath space per apache#2059); replace-mode symlink test asserts the canonical target is written - filesystem-worker-client: writable-roots test back to main narrow dirname form; entry normalizer resolves auto scope; drop symlink pin comparisons - storage: buildSessionHeader/normalizeSessionHeader persist and validate editingProtocol; SQLite session-store regression test
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
LocalWorkspaceExecutordecided path containment by comparing a realpath'd session cwd against a candidate that was onlyresolved. The two sides lived in different path spaces, so any session cwd under a symlink rejected every legitimate absolute path inside the workspace: a model that wrote by absolute path could not touch its own workspace. macOS hits this always (os.tmpdir()returns/var/folders/…, whose realpath is/private/var/folders/…), and so does anyone whose workspace root is reached through a symlink; Linux CI stayed green only because it happens to have no such symlink in the path.The fix canonicalises the candidate before the check, through its deepest existing ancestor so a not-yet-created target still resolves its parent's symlinks. Both guarantees are kept and now fall out of a single rule instead of two staged checks:
Canonicalisation and the containment assertion are separated so the write-lock key can share the former without the latter. Without that,
data.txtand/var/…/link/data.txtproduced two different keys for one file, so concurrent Edits took different locks and clobbered each other.The containment error now prints both the caller's input and what it resolved to; previously it printed the realpath'd root next to the raw input, which is what made the failure hard to read.
realpathAllowMissingmoves intopath-containment.ts, the owner of the containment invariant, replacing the two existing copies insandbox-boundary-path.tsandfilesystem-worker/operations.ts.Root cause
resolveExistingInsideCwd/resolveWritableInsideCwdcomputedroot = await fs.realpath(cwd)but left the caller's absolute path unnormalised, then compared them withisPathInside.Audit of the same class in
packages/runtime: the sandboxed filesystem-worker's read path is already single-space —builtin-tools.tscanonicalises the cwd withrealpathSyncand the client rewritesoperation.pathto the normalisedenforcementPath, sooperations.tscompares two canonical paths (its write path had a separate hole, below).globFilesreceives the resolved (canonical) base fromresolveExistingPath, andgrepFilesreceives an absolute canonical search path, so neither needs its own normalisation.packages/headless's isolated-workspace adapter is a deliberate lexical-only preflight and compares two lexical paths, so it is unaffected.writeLockKeywas the one remaining instance in the executor and is fixed here.Security
Three containment holes are closed on top of the path-space fix. Each was reproduced before it was fixed.
Writing through a symlink out of the workspace. The writable resolver checked the parent directory but returned the unresolved leaf, so a symlink inside the workspace pointing out of it was followed by the write. Reads already rejected this. Now rejected for writes too — the caller-visible behaviour change on this PR.
Dangling symlinks.
realpathfails ENOENT on a link whose target does not exist, so canonicalisation treated such a link as a plain missing leaf and returned the link's own path — which reads as contained, while the write lands on the link's target outside the workspace.realpathAllowMissingnow reads and follows the link by hand.realpathreports a cycle as ELOOP, and the kernel caps its own symlink traversal well below the helper's hop cap, so the walk always terminates by rejecting. The other two consumers of the helper gain the same accuracy: a dangling link normalises to the path a write would actually reach, which is the path the sandbox boundary should approve.The worker authorising an unresolved write target.
resolveWritableAllowedin the filesystem worker realpaths the parent of a missing target and then authorises the unresolved candidate, so a dangling link inside the root passed its boundary check while the write landed outside. Reaching it takes a request whose boundary andexpectedTargetdisagree, which the in-repo client never sends — but the worker is a separate process that re-checks everything precisely so it does not have to trust its caller. It now authorises the followed path, the same oneassertTargetUnchangedalready pins the request to.Collapsing the two resolvers also dropped the containment check the old existing-path variant ran on its final
fs.realpathresult; it is restored, with a comment saying why it must not be deleted. The candidate is already canonical, so the two can only diverge when a segment that was missing during canonicalisation becomes a symlink before the realpath — narrow, and no deterministic test can drive that race, so nothing fails if it is removed.Verification
packages/runtime:npm run build && node --test "dist/**/*.test.js"→ 3090 pass / 0 fail / 9 skipped on macOS. Before the fix: 3076 pass / 4 fail (the four containment tests inbuiltin-tools.test.ts).file tools stay usable when the session cwd is reached through a symlink(Read/Write/Edit/Glob/Grep accepted through the link; outside paths, an escaping file symlink, an escaping directory symlink and an escaping dangling symlink still rejected) andconcurrent Edits through a symlinked cwd serialize on one key.realpathAllowMissinggets direct unit tests inpath-containment.test.ts(symlinked ancestor, missing trailing segments, dangling link, dangling chain, ENOTDIR, pathological chain, cycle → ELOOP); it is the canonicalisation authority for three subsystems and previously had none.denies a write through a dangling symlink the boundary does not coverinfilesystem-worker.test.ts.writeLockKeychange re-breaks the lock test, and reverting only the dangling-link hop re-breaks three tests.npm run build,npm run typecheck,npm run lint,npm run format:check→ clean.npm run test -w @maka/headless→ 1339 pass / 0 fail;npm run test -w @maka/desktop→ 1585 pass / 0 fail.main(ef3905a3) and re-verified end to end.